Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: implement security auditing checks #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ci: implement security auditing checks #139

wants to merge 1 commit into from

Conversation

nrdxp
Copy link
Contributor

@nrdxp nrdxp commented Oct 7, 2024

Description

uses cargo-audit and the RustSec Advisory DB

see: https://rustsec.org/

The config file is pretty minimal to start.

It is possible to add irrelevant (to us) issues in the ignore list though. Issues can be reviewed in a nice formatted form after
the Action has run in a follow up job.

It is still debatable how we would like to run this. On every push for PRs (as is configured now), or perhaps simply just on a
regular interval (currently every day at midnight). We probably don't want to make this a hard requirement to merge PRs though, as it may take dedicated time and effort to:

  • actually determine if the issue is relevant to our use of the code
  • bump the dependencies, which may require unrelated code changes.

However, perhaps we have a regular review, in the form of a meeting or just async, to ensure we never get too far behind. Currently, we have 3 vulnerabilities that are reported and several warnings.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages.
  • New tests are added if needed and existing tests are updated.
  • Relevant logging and metrics added
  • CI passes. See note on CI.
  • Any changes are noted in the changelog.md for affected crate
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG Partner Chains developers to do this
for you.

@skylar-simoncelli
Copy link
Contributor

Can this be migrated into ci.yml instead of another new workflow? I already have plans to remove all other superflous workflows and narrow down to only ci.yml and cd.yml, and even then I would like to eventually merge these two together

The below PR would move this step into ci.yml and execute in every pre-merge, post-merge and workflow_dispatch run:

#149

This way it would be running several times per day, which should remove the need for the daily cron trigger

@nrdxp
Copy link
Contributor Author

nrdxp commented Oct 9, 2024

Its possible, however, if we want to maintain the chron job, I'm not sure it is. We may want to run this exclusively as a chron job to avoid the noise of running for every PR push, perhaps.

`cargo-audit` and the RustSec Advisory DB

see: https://rustsec.org/
@skylar-simoncelli
Copy link
Contributor

Sorry forgot to approve. PR looks good however you decide to implement.

I would have a preference for just including the job in ci.yml to run on PR as it only takes 30 seconds and could potentially highlight new vulnerabilities introduced in a branch before they are merged

Copy link
Contributor

@AmbientTea AmbientTea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this can stop PRs that change Cargo.toml from being merged even if the audit failure doesn't come from their changes, just some committed version having a new vulnerability. Ideally we'd be able to merge changes that don't introduce issues, but I don't think that's possible to do with this tool. Perhaps we should just run this on cron?

@nrdxp
Copy link
Contributor Author

nrdxp commented Oct 21, 2024

It looks like this can stop PRs that change Cargo.toml from being merged even if the audit failure doesn't come from their changes, just some committed version having a new vulnerability. Ideally we'd be able to merge changes that don't introduce issues, but I don't think that's possible to do with this tool. Perhaps we should just run this on cron?

As is, I did not make this a required check so it wouldn't block merge in that way. However, I've also thought about just setting a cron job (in this cut of the PR it is both). The downside seems like less visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants